v2/pkg/astvalidation/operation_rule_values.go (3)

472-538: Consider extracting the variable nullable check logic.

The nullable variable validation logic (lines 508-535) within objectValueViolatesOneOf is quite lengthy and could benefit from being extracted into a separate helper method for better readability and testability.

Consider refactoring the nullable variable checking into a helper method:

 func (v *valuesVisitor) objectValueViolatesOneOf(objectValue ast.Value, defRef int) bool {
 	def := v.definition.InputObjectTypeDefinitions[defRef]
 	// Check if the input object type has @oneOf directive
 	if !def.HasDirectives {
 		return false
 	}
 	hasOneOfDirective := def.Directives.HasDirectiveByName(v.definition, "oneOf")
 	if !hasOneOfDirective {
 		return false
 	}
 
 	fieldRefs := v.operation.ObjectValues[objectValue.Ref].Refs
 	if len(fieldRefs) != 1 {
 		objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
 		v.Report.AddExternalError(operationreport.ErrOneOfInputObjectFieldCount(objName, len(fieldRefs), objectValue.Position))
 		return true
 	}
 
-	type nullableVar struct {
-		fieldRef              int
-		fieldValue            ast.Value
-		variableDefinitionRef int
-	}
-	var nullableVars []nullableVar
-
-	for _, fieldRef := range fieldRefs {
-		fieldValue := v.operation.ObjectFieldValue(fieldRef)
-
-		if fieldValue.Kind == ast.ValueKindNull {
-			objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
-			fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
-			v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position))
-			return true
-		}
-
-		if fieldValue.Kind == ast.ValueKindVariable {
-			// For variables, check if the variable type is nullable
-			variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref)
-			if !ok {
-				continue
-			}
-
-			// Collect nullable variables
-			if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull {
-				nullableVars = append(nullableVars, nullableVar{
-					fieldRef,
-					fieldValue,
-					variableDefinitionRef})
-			}
-		}
-	}
-
-	// If exactly one field, but it's a nullable variable, report that error
-	if len(nullableVars) > 0 {
-		violation := nullableVars[0]
-		objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
-		fieldName := v.operation.ObjectFieldNameBytes(violation.fieldRef)
-		variableName := v.operation.VariableValueNameBytes(violation.fieldValue.Ref)
-		v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable(
-			objName, fieldName, variableName, violation.fieldValue.Position,
-			v.operation.VariableDefinitions[violation.variableDefinitionRef].VariableValue.Position))
-		return true
-	}
+	// Check single field value
+	fieldRef := fieldRefs[0]
+	fieldValue := v.operation.ObjectFieldValue(fieldRef)
+
+	if fieldValue.Kind == ast.ValueKindNull {
+		objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
+		fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
+		v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position))
+		return true
+	}
+
+	if v.isNullableVariableViolation(fieldRef, fieldValue, defRef) {
+		return true
+	}
 
 	return false
 }
+
+func (v *valuesVisitor) isNullableVariableViolation(fieldRef int, fieldValue ast.Value, defRef int) bool {
+	if fieldValue.Kind != ast.ValueKindVariable {
+		return false
+	}
+
+	// For variables, check if the variable type is nullable
+	variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref)
+	if !ok {
+		return false
+	}
+
+	// Check if nullable
+	if v.operation.Types[variableTypeRef].TypeKind == ast.TypeKindNonNull {
+		return false
+	}
+
+	// Report nullable variable violation
+	objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
+	fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
+	variableName := v.operation.VariableValueNameBytes(fieldValue.Ref)
+	v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable(
+		objName, fieldName, variableName, fieldValue.Position,
+		v.operation.VariableDefinitions[variableDefinitionRef].VariableValue.Position))
+	return true
+}

---

433-436: Hooking @OneOf into input-object validation looks correct.

The placement after duplicate-field checks is sensible. Note: in variables validation the @OneOf check runs before the “unknown field” check, while here it runs after. If you want consistent error precedence across both paths, consider moving this block before “unknown field” checks.

---

472-538: Early-return on first nullable-variable found to avoid allocations.

You don’t need to collect all nullable variables; returning on the first violation is enough and avoids slice allocation.

Apply this diff:

 func (v *valuesVisitor) objectValueViolatesOneOf(objectValue ast.Value, defRef int) bool {
@@
-    type nullableVar struct {
-        fieldRef              int
-        fieldValue            ast.Value
-        variableDefinitionRef int
-    }
-    var nullableVars []nullableVar
-
     for _, fieldRef := range fieldRefs {
         fieldValue := v.operation.ObjectFieldValue(fieldRef)
 
         if fieldValue.Kind == ast.ValueKindNull {
             objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
             fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
             v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position))
             return true
         }
 
         if fieldValue.Kind == ast.ValueKindVariable {
             // For variables, check if the variable type is nullable
             variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref)
             if !ok {
                 continue
             }
 
-            // Collect nullable variables
-            if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull {
-                nullableVars = append(nullableVars, nullableVar{
-                    fieldRef,
-                    fieldValue,
-                    variableDefinitionRef})
-            }
+            if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull {
+                objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
+                fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
+                variableName := v.operation.VariableValueNameBytes(fieldValue.Ref)
+                v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable(
+                    objName, fieldName, variableName, fieldValue.Position,
+                    v.operation.VariableDefinitions[variableDefinitionRef].VariableValue.Position))
+                return true
+            }
         }
     }
 
-    // If exactly one field, but it's a nullable variable, report that error
-    if len(nullableVars) > 0 {
-        violation := nullableVars[0]
-        objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
-        fieldName := v.operation.ObjectFieldNameBytes(violation.fieldRef)
-        variableName := v.operation.VariableValueNameBytes(violation.fieldValue.Ref)
-        v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable(
-            objName, fieldName, variableName, violation.fieldValue.Position,
-            v.operation.VariableDefinitions[violation.variableDefinitionRef].VariableValue.Position))
-        return true
-    }
-
     return false
 }

v2/pkg/variablesvalidation/variablesvalidation.go (2)

375-421: Consider improving variable naming and field counting logic.

While the implementation is functionally correct, there are a few areas that could be improved:

1. The variable name firstNullField could be more descriptive (e.g., nullFieldName)
2. The obj.Visit iteration could be optimized to early exit once a null field is found
3. The field counting could be simplified since obj.Len() already provides the total count

Consider these improvements for better clarity and efficiency:

 func (v *variablesVisitor) violatesOneOfConstraint(inputObjectDefRef int, jsonValue *astjson.Value, typeName []byte) bool {
 	def := v.definition.InputObjectTypeDefinitions[inputObjectDefRef]
 
 	// Check if the input object type has @oneOf directive
 	if !def.HasDirectives {
 		return false
 	}
 	hasOneOfDirective := def.Directives.HasDirectiveByName(v.definition, "oneOf")
 	if !hasOneOfDirective {
 		return false
 	}
 
 	// Count all fields in the JSON object
 	obj := jsonValue.GetObject()
 	totalFieldCount := obj.Len()
 
 	// Prioritize the count error
 	if totalFieldCount != 1 {
 		variableContent := string(jsonValue.MarshalTo(nil))
 		v.err = v.newInvalidVariableError(
 			fmt.Sprintf(`%s; OneOf input object "%s" must have exactly one field provided, but %d fields were provided.`,
 				v.invalidValueMessage(string(v.currentVariableName), variableContent),
 				string(typeName), totalFieldCount))
 		return true
 	}
 
-	// Try to find at least one null field.
-	var firstNullField []byte
+	// Check if the single field has a null value
+	var nullFieldName []byte
 	obj.Visit(func(key []byte, val *astjson.Value) {
-		if firstNullField == nil && val.Type() == astjson.TypeNull {
-			firstNullField = key
+		if val.Type() == astjson.TypeNull {
+			nullFieldName = key
 		}
 	})
 
-	// Check if we have exactly one field, and it's non-null
-	if firstNullField == nil {
+	// If the field is non-null, the constraint is satisfied
+	if nullFieldName == nil {
 		return false
 	}
 
 	variableContent := string(jsonValue.MarshalTo(nil))
 	v.err = v.newInvalidVariableError(
 		fmt.Sprintf(`%s; OneOf input object "%s" field "%s" value must be non-null.`,
 			v.invalidValueMessage(string(v.currentVariableName), variableContent),
-			string(typeName), string(firstNullField)))
+			string(typeName), string(nullFieldName)))
 	return true
 }

---

375-421: Include the JSON path in @OneOf error messages for better DX.

Other errors include the variable path (at "..."). Adding it here improves consistency and debuggability, especially for lists.

Apply this diff:

 func (v *variablesVisitor) violatesOneOfConstraint(inputObjectDefRef int, jsonValue *astjson.Value, typeName []byte) bool {
@@
-    if totalFieldCount != 1 {
-        variableContent := string(jsonValue.MarshalTo(nil))
-        v.err = v.newInvalidVariableError(
-            fmt.Sprintf(`%s; OneOf input object "%s" must have exactly one field provided, but %d fields were provided.`,
-                v.invalidValueMessage(string(v.currentVariableName), variableContent),
-                string(typeName), totalFieldCount))
+    if totalFieldCount != 1 {
+        variableContent := string(jsonValue.MarshalTo(nil))
+        var path string
+        if len(v.path) > 1 {
+            path = fmt.Sprintf(` at "%s"`, v.renderPath())
+        }
+        v.err = v.newInvalidVariableError(
+            fmt.Sprintf(`%s%s; OneOf input object "%s" must have exactly one field provided, but %d fields were provided.`,
+                v.invalidValueMessage(string(v.currentVariableName), variableContent),
+                path, string(typeName), totalFieldCount))
         return true
     }
@@
-    variableContent := string(jsonValue.MarshalTo(nil))
-    v.err = v.newInvalidVariableError(
-        fmt.Sprintf(`%s; OneOf input object "%s" field "%s" value must be non-null.`,
-            v.invalidValueMessage(string(v.currentVariableName), variableContent),
-            string(typeName), string(firstNullField)))
+    variableContent := string(jsonValue.MarshalTo(nil))
+    var path string
+    if len(v.path) > 1 {
+        path = fmt.Sprintf(` at "%s"`, v.renderPath())
+    }
+    v.err = v.newInvalidVariableError(
+        fmt.Sprintf(`%s%s; OneOf input object "%s" field "%s" value must be non-null.`,
+            v.invalidValueMessage(string(v.currentVariableName), variableContent),
+            path, string(typeName), string(firstNullField)))
     return true
 }

v2/pkg/astvalidation/operation_validation_test.go (2)

5255-5260: Clarify the @OneOf directive description.

Consider stating the behavioral contract in the docstring to aid readers/tools that surface descriptions via introspection.

Apply this diff:

-"""
-The @oneOf built-in directive is used within the type system definition language
-to indicate an Input Object is a OneOf Input Object.
-"""
+"""
+The @oneOf built-in directive marks an input object as a OneOf Input Object.
+Exactly one field must be provided and its value must be non-null at runtime.
+All fields defined within a @oneOf input must be nullable in the schema.
+"""
 directive @oneOf on INPUT_OBJECT

---

3108-3171: Good coverage; rename duplicate test and relax brittle assertions.

• Two tests share the name “oneOf with null field”; rename the second for clarity.
• The “oneOf with one null field” and “oneOf with two fields” cases assert the exact count suffix (“but 2 fields were provided”), which may be brittle if validation ordering changes. Matching only the invariant prefix is safer.

Apply this diff:

-				t.Run("oneOf with null field", func(t *testing.T) {
+				t.Run("oneOf with both fields null", func(t *testing.T) {
 					run(t, `
 						mutation oneOfWithNoFields {
 							addPet(pet: { cat: null, dog: null }) {
 								name
 							}
 						}`, Values(), Invalid,
 						withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided, but 2 fields were provided`))
 				})

-						}`, Values(), Invalid,
-						withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided, but 2 fields were provided`))
+						}`, Values(), Invalid,
+						withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided`))
 				})

-						}`, Values(), Invalid,
-						withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided, but 2 fields were provided`))
+						}`, Values(), Invalid,
+						withValidationErrors(`OneOf input object "PetInput" must have exactly one field provided`))
 				})

Optionally, add a positive list case to complement the negative one:

 				t.Run("list of oneOf with nullable variable", func(t *testing.T) {
 					run(t, `
 						mutation listOfOneOfWithNullableVariable($dog: DogInput) {
 							addPets(pets: [{ dog: $dog }]) {
 								name
 							}
 						}`, Values(), Invalid,
 						withValidationErrors(`OneOf input object "PetInput" field "dog" cannot use nullable variable "$dog". Variables used in oneOf fields must be non-nullable`))
 				})
+				t.Run("list of oneOf with non-null variable", func(t *testing.T) {
+					run(t, `
+						mutation listOfOneOfWithNonNullVariable($dog: DogInput!) {
+							addPets(pets: [{ dog: $dog }]) {
+								name
+							}
+						}`, Values(), Valid)
+				})

v2/pkg/variablesvalidation/variablesvalidation_test.go (1)

1494-1712: Great coverage for @OneOf; consider a couple more edge cases.

• Looks solid: valid/invalid, nesting, lists, and variable-content-enabled cases are well covered.
• Optional: Add a case where a list element is an empty object {} (field count = 0) to ensure the per-element error is surfaced with list context.
• Optional: Add a case with an unknown field alongside a valid field on a @OneOf object to document precedence vs. other validations (unknown field vs. oneOf count). This can help align with operation value validation precedence.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

v2/pkg/variablesvalidation/variablesvalidation.go (1)

375-433: Variables-side @OneOf enforcement looks correct; minor polish suggested.

• Extract directive name "oneOf" to a shared constant for reuse and to avoid typos.
• Tiny nit: rename “Count all fields in the JSON object” comment to “Count provided fields” for clarity.

v2/pkg/variablesvalidation/variablesvalidation_test.go (1)

1495-1711: Add operation-validation test for @OneOf with nullable variable
No existing coverage for pet(input:{cat: $cat}) where $cat: String; add a test in v2/pkg/astvalidation/operation_validation_test.go exercising ErrOneOfInputObjectNullableVariable.

v2/pkg/astvalidation/operation_validation_test.go (1)

5068-5071: LGTM: PetInput annotated with @OneOf

Fields are nullable as required by the oneOf contract.

Consider adding a negative test (elsewhere) that asserts schema-level rejection if a @OneOf input defines any Non‑Null field, to guard the schema rule too.

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

v2/pkg/astvalidation/operation_rule_values.go (1)

484-522: Consider simplifying the field iteration.

Since you've already verified len(fieldRefs) == 1 at line 485, the loop on lines 491-519 will only execute once. You could simplify by accessing fieldRefs[0] directly:

 fieldRefs := v.operation.ObjectValues[objectValue.Ref].Refs
 if len(fieldRefs) != 1 {
     objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
     v.Report.AddExternalError(operationreport.ErrOneOfInputObjectFieldCount(objName, len(fieldRefs), objectValue.Position))
     return true
 }

-for _, fieldRef := range fieldRefs {
+// Exactly one field is present (verified above)
+fieldRef := fieldRefs[0]
+{
     fieldValue := v.operation.ObjectFieldValue(fieldRef)
     
     if fieldValue.Kind == ast.ValueKindNull {
         objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
         fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
         v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullValue(objName, fieldName, fieldValue.Position))
         return true
     }
     
     if fieldValue.Kind == ast.ValueKindVariable {
         // For variables, check if the variable type is nullable
         variableDefinitionRef, variableTypeRef, _, ok := v.operationVariableType(fieldValue.Ref)
         if !ok {
             continue
         }
         
         // Collect nullable variables
         if v.operation.Types[variableTypeRef].TypeKind != ast.TypeKindNonNull {
             objName := v.definition.InputObjectTypeDefinitionNameBytes(defRef)
             fieldName := v.operation.ObjectFieldNameBytes(fieldRef)
             variableName := v.operation.VariableValueNameBytes(fieldValue.Ref)
             v.Report.AddExternalError(operationreport.ErrOneOfInputObjectNullableVariable(
                 objName, fieldName, variableName, fieldValue.Position,
                 v.operation.VariableDefinitions[variableDefinitionRef].VariableValue.Position))
             return true
         }
     }
 }

 return false

However, the current loop-based approach is defensive and consistent with patterns used elsewhere in this file, so this is purely optional.

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro